Repository navigation
ci: check-empty-catch gates a discarded .catch(() => {}) (void or a reason); fix the 23 sites (#213) - #268
Conversation
…eason); fix the 23 sites (ThinkFlowLab#213)
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7afe8e54f4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| - A `catch` that swallows the error says why in a comment inside the block (or handles it); `check-empty-catch` in `.github/scripts/` fails the build on a bare one (#205). Text inside comments, strings, templates and regex literals is not code, so a `catch {}` written there is never reported (#213). | ||
| - The `void p.catch(() => {})` promise form is a separate fire-and-forget idiom and test-helper retries stay out of the gate's scope. | ||
| - The promise form follows the same rule (#213): a `.catch` whose handler does nothing (`() => {}`, `() => undefined`, `() => null`) and whose result is dropped (a statement, or an `await` statement) needs a `void` prefix or a one-line reason comment (in the handler, after the call, or on the line above). A handler that returns a fallback value that is *used* (`const x = await p.catch(() => undefined)`, an argument, a `return`, an assignment) stays allowed. `void` suits fire-and-forget; an `await` that waits for the work to settle takes the reason comment. |
There was a problem hiding this comment.
With ARCH_DOCS_BASE=275e957f3aee2ff4ab9a9b07f33602cdd64e2f25, node core/scripts/check-arch-docs.mjs fails because this commit changes owned core-client, core-host, social, and tree files but updates none of docs/subsystems/{core-client,core-host,social,tree}.md, and its message has no Docs-not-needed: trailer. Updating only this general standards page therefore leaves npm run verify blocked; update the affected subsystem docs or add the trailer.
AGENTS.md reference: AGENTS.md:L94-L94
Useful? React with 👍 / 👎.
| if (depth === 0) return { start: i + 1, argument: true } | ||
| depth -= 1 | ||
| } else if (depth === 0) { | ||
| if (char === ';' || char === '{' || char === '}') return { start: i + 1, argument: false } |
There was a problem hiding this comment.
When a fallback is used as an object property, for example use({ value: await load().catch(() => undefined) }), the backward walk stops at the object literal's { here and marks the catch as a dropped statement; scanPromiseSource consequently reports a violation even though the fallback value is passed to use. This contradicts the newly documented used-value exemption and will reject valid production code whenever the fallback is wrapped in an object, so object-literal braces need to be balanced or the enclosing value context otherwise retained.
Useful? React with 👍 / 👎.
| const char = code[i] | ||
| if (char === ')' || char === ']') depth += 1 | ||
| else if (char === '(' || char === '[') { | ||
| if (depth === 0) return { start: i + 1, argument: true } |
There was a problem hiding this comment.
For a standalone grouped expression such as (save().catch(() => {})) (and an ASI-safe ;(save().catch(() => {}))), this branch treats the unmatched grouping ( as proof that the result is an argument and returns no violation. The result is still dropped, so harmless grouping bypasses the new required void/reason gate; only actual argument, condition, or other value-consuming contexts should be exempted.
Useful? React with 👍 / 👎.
Fixes #213 (item 2 landed in #266; this is item 1, decided:
voidor a one-line reason on a discarded.catch(() => {}); a used fallback value stays allowed).Rule. A
.catchwhose handler does nothing (() => {},() => undefined,() => null) and whose result is dropped (a statement or anawaitstatement) needs avoidprefix or a one-line reason comment (in the handler, after the call on its line, or on the line above). Used values stay allowed:const x = await p.catch(() => undefined), an argument, areturn, an assignment, an arrow's expression body, a ternary.Checker.
check-empty-catch.mjsgetsscanPromiseSource/scanPromiseTreeon the same masked source (comments and literals never match). It finds the statement the call sits in (walking back over balanced brackets and multi-line chains) and classifies it. 83 idle handlers exist in the shipped sources; 23 were discarded withoutvoidor a reason, the rest arevoid, values or already commented.Sites. 22 awaited swallows got a reason in the handler (rollback is best effort, file may not exist, a failed pull is fixed by the next one, ...); 2 fire-and-forget chains got
void(memory-panel,voice). No behaviour change.Tests: discarded forms, multi-line chains, callbacks in the chain,
void/reason forms (handler, trailing, line above), used values, literals, and the shipped sources. Docs:docs/code-standards.md. Patch topics: eight upstream files newly edited are listed under0001-core-fixes. Nothing deployed.